docs: explain LAMMPS exploration extra files - #393
Conversation
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #393 +/- ##
==========================================
+ Coverage 84.43% 84.45% +0.01%
==========================================
Files 104 104
Lines 6110 6110
==========================================
+ Hits 5159 5160 +1
+ Misses 951 950 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Document the input_extra_files path for DP-ZBL tables and verify that normalized lmp-md task groups copy each file into every task. Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
b472b34 to
23dee80
Compare
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thanks for finishing this bookkeeping - Closes #304 is correct. The feature landed in eb779cb (#306, 2025-08-29), six days after #304 was filed, but #306's body said "fix issue #305" (an unrelated resubmit crash), so #304 never auto-closed. #306 also claimed "The docs were updated too" while git show eb779cb -- docs/ is empty, so the remaining gap really is prose. I traced the mechanism end to end and it works: npt_task_group.py stores the basename and adds the file to every task, prep_lmp.py writes every key of files() into the task dir, and run_lmp.py symlinks everything under the task path into the LAMMPS work dir before the freeze step. The docs are accurate.
One thing to fix, with two surfaces.
The new test's fixture is degenerate in exactly the two dimensions the new paragraph makes claims about. The paragraph asserts (a) the file is copied "into every exploration task directory" and (b) it "is available there by its basename". The single new test pins neither, for two independent reasons, so one fixture change closes both. Details are in the two inline comments.
Evidence for (b), since it is the less obvious one - I reverted just the basename call in dpgen2/exploration/task/npt_task_group.py:
- self.input_extra_files = [Path(ii).name for ii in input_extra_files]
+ self.input_extra_files = [str(ii) for ii in input_extra_files]
All 5 tests in tests/exploration/test_make_task_group_from_config.py still pass. For contrast, deleting the add_file loop that the test does cover fails it with KeyError: 'SiC_ZBL.txt', so the test is not vacuous - it just stops one step short of the property the docs advertise.
A suggested fixture that closes both at once: write the file into a subdirectory, pass sub/SiC_ZBL.txt (so basename stripping is observable), set "Ts": [100, 200] (so more than one task exists), and assert over every task in the group rather than tgroup[0].
Non-blocking notes, recorded so nobody has to rediscover them:
- The branch no longer merges.
git merge-tree --write-tree upstream/master 23dee80conflicts indocs/input.md, because #372 added a#### PLUMED CV candidate filteringsubsection into the same gap. On rebase the new paragraph should stay immediately after then_sampleparagraph, before that####heading - appending it afterwards would make it read as part of the PLUMED subsection. input_extra_filesalso exists oncustomized-lmp-template, where it means something different (files for the shell commands) and is genuinely not propagated: the innerlmp_tgroup.set_lmp(...)in_make_customized_task_groupis called without it, so those files never reach the generated LAMMPS tasks. Your paragraph anchors totask_group[lmp-md]so it is not wrong, but that is a real separate bug worth its own issue.- The
lmp-templatehalf of #306 is still uncovered: deleting itsadd_fileloop leaves all 62 tests undertests/exploration/green. Out of scope here, just noting it did not come with #306 either.
Two things I checked and am explicitly not raising, so they do not get re-opened: assets/dp/SiC_ZBL.txt not existing is fine - the whole example block uses illustrative paths (/file/prefix, relpath/to/confs/*, template.lammps); and writing the fixture into the CWD matches how every test under tests/exploration/ has worked since 2022.
| def test_npt_copies_input_extra_files_to_each_task(self): | ||
| config = { | ||
| **self.config_npt, | ||
| "input_extra_files": [str(self.extra_file)], | ||
| } | ||
| tgroup = make_lmp_task_group_from_config( | ||
| self.numb_models, self.mass_map, config | ||
| ) | ||
| tgroup.set_conf(["LAMMPS configuration"]) | ||
| tgroup.make_task() | ||
|
|
||
| self.assertEqual( | ||
| tgroup[0].files()[self.extra_file.name], | ||
| "ZBL table content\n", | ||
| ) |
There was a problem hiding this comment.
This builds exactly one task, so the name ..._to_each_task and the docs claim "every exploration task directory" are both unverified here.
self.config_npt is {"type": "lmp-md", "Ts": [100]} with no press, so set_md leaves self.press = [None]; set_conf(["LAMMPS configuration"]) defaults n_sample to len(conf_list) == 1. make_task's itertools.product(confs, self.temps, self.press) is therefore 1x1x1. Since the assertion only reads tgroup[0], a regression that copied the extra file into only the first task of a multi-task group would leave this green.
"Ts": [100, 200] plus an assertion over every task in the group would pin it. I confirmed the real behaviour is correct - with two temperatures and two configurations all four tasks do carry the file - so this is about the test, not the code.
|
|
||
| class TestMakeLmpTaskGroupFromConfig(unittest.TestCase): | ||
| def setUp(self): | ||
| self.extra_file = Path("SiC_ZBL.txt") |
There was a problem hiding this comment.
The fixture path has no directory component, which makes the basename behaviour invisible to the test.
Because this is the bare name SiC_ZBL.txt, Path(ii).name and str(ii) produce the identical string in npt_task_group.py, and replacing one with the other leaves all 5 tests passing. That is the specific property the new paragraph asserts ("available there by its basename") and the specific property the new example depends on - assets/dp/SiC_ZBL.txt is listed but referenced downstream as SiC_ZBL.txt.
Writing the fixture into a subdirectory and passing sub/SiC_ZBL.txt would make the stripping observable.
Summary
Tests
Closes #304
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh